Expose attachInterruptArg in Arduino.h and updated base for functional interrupts#6047
Expose attachInterruptArg in Arduino.h and updated base for functional interrupts#6047dok-net wants to merge 5 commits intoesp8266:masterfrom
Conversation
|
@d-a-v : could #6039 please take into account all the work and research I've done on fixing the interrupt handling? Please remember, that at heart, #6038 is the cleanest ISR vs. ICACHE_RAM safe implementation at this present stage I have to offer, the caveat being that the user cannot mix functional and non-functional attaches and detaches without care. #6003 provides this extra ease, at the expense of copied struct definition, dependency inversion between FunctionalInterrupt and core_esp8266_wiring_digital and the extra functional field only for the purpose of supporting FunctionalInterrupt. My take is still to go for clean dependencies over ease of use :-) |
|
@dok-net Thanks for your work, |
|
@dok-net #6039 is orthogonal to your PRs #6003 #6038 #6047. |
|
Dear @d-a-v , no, stepping on my toes is much to hard a word, I am just a little panicky that #6039 might cause a merge mess for me, I only wanted to point that out. As far a bug fix vs. feature is concerned, I've created issue report #6049 to hopefully motivate sufficiently why this PR is a bug fix and should be in the very next bugfix release. |
ec68d2a to
c728b16
Compare
Yes, it should work this way (in git).
I couldn't quite wrap my head around the irom.text etc. meddling, and unfortunately the above mentioned comment features incomplete code and I can't figure it quite out either - I would like to trust that the pertaining parts of std::function<void()> are either inlined or linker-edited into IRAM as per #5922, and that is sufficient for #6047, unless someone attaches/detaches ISRs from inside executing ISRs, on which opinions vary widely :-) |
|
@hreintke I almost can't resist giving an answer that's just as cryptic :-) |
|
@hreintke, There's no cache miss handler code. Cache fetches are done completely in HW. If the HW state machine can't engage the SPI bus to do the read you get a nonrecoverable HW exception at that point and crash hard. If it can access the bus, it will work since interrupts are just normal codepaths and nothing fancy on these machines. As a debug only method, you might be able to do something fishy to the SPI bus at the start of our interrupt dispatch code and then undo it on return. Then panics will occur should ISR code attempt to go out to flash. But that's debug only and it will break SPI, WiFi persistence, and anything else that access flash as a data store. I think it's probably overkill, though. Even the ISR-in-RAM check is a bit pedantic since it is possible for an app to never access flash as data store, or an app could disable IRQs before doing so (and killing any IRQ generated stuff like PWM, etc.). |
58d5a45 to
4914318
Compare
5c09bcc to
87a9b86
Compare
|
@d-a-v It seems you've some time at your hands today to talk - I've a question: What is the state of the IRAM linker segment manipulations w.r.t. std::function now? I have hacked a fix for EspSoftwareSerial (master HEAD) to use that in the meantime until my bigger PRs get reviewed and merged, and somehow it's working using the FunctionalInterrupts API instead of the C-style attachInterruptArg (in my PRs). Before, it seemed unstable. Also, a standalone test in libraries/FunctionalInterrupt/ that I had seen failing, as I believed due to flash accessing during interrupt service - runs fine. As much as I understood it, std::function<void(void)> are OK, does this depend on the captures in any way? |
Since #5922, all functional effective code is moved into IRAM. You can also more easily check from a running sketch whether a lambda is linked IRAM with this trick. |
924d498 to
2600d30
Compare
17b19f9 to
290ce5f
Compare
e046c9f to
61661c4
Compare
|
@d-a-v I was asked by @devyte to test and improve the performance of this patch, such that it is no worse for void(*)(void) ISRs over before. I am now seing a major improvement in performance in my test, but I could have implemented a horrible bug :-) Could you please read |
|
Some test results. Master including this fix has following memory use for small test case using void(*)(void) ISRs: It performs two cascaded IRQs with an avg. latency/CPU_cycles = 961. Just switching core to this branch, the results are: Performance is: avg. Latency/CPU_cycles = 891. Next, changing the test case from plain function pointer to std::function for a void(int) curried with an integer value, this is result: Performing at avg. Latency/CPU_cycles = 929. The sketch: |
c59d056 to
1c45e6f
Compare
1c45e6f to
3f46269
Compare
|
Latest master compared to rebased PR branch. PR: Running: Sketch that compiles both in master and PR: EspSoftwareSerial, off-the-shelf loopback.ino example with |
c25b08c to
4bbb23b
Compare
|
Here's the latest in performance numbers and what the new Delegate class can do for interrupts. See the comments with the measurements below in this working example. Connect D5 to D8, D6 to D7. Pending pushing the updated PR to GitHub: #include <interrupts.h>
uint32_t start;
uint8_t toggle_d8 = HIGH;
uint8_t toggle_d6 = HIGH;
void IRAM_ATTR irq_d7(void* x)
{
toggle_d8 ^= HIGH;
digitalWrite(D8, toggle_d8);
}
void IRAM_ATTR irq_d7_v()
{
irq_d7(nullptr);
}
uint32_t sum_delay = 0;
constexpr uint32_t MAXREPEATS = 1000;
uint32_t repeats = 0;
void IRAM_ATTR irq_d5(void* x)
{
auto cpuCycle = ESP.getCycleCount();
sum_delay += cpuCycle - start;
++repeats;
start = cpuCycle;
if (repeats < MAXREPEATS)
{
toggle_d6 ^= HIGH;
digitalWrite(D6, toggle_d6);
}
}
void IRAM_ATTR irq_d5_v()
{
irq_d5(nullptr);
}
void setup()
{
Serial.begin(9600);
pinMode(D8, OUTPUT);
digitalWrite(D8, toggle_d8);
pinMode(D7, INPUT_PULLUP);
pinMode(D6, OUTPUT);
digitalWrite(D6, toggle_d6);
pinMode(D5, INPUT_PULLUP);
attachInterrupt(D7, irq_d7_v, CHANGE);
attachInterrupt(D5, irq_d5_v, CHANGE);
// master / 2.6.2: avg. Latency/us = 12.29
// PR #6047 + Delegate: avg. Latency/us = 11.42
//attachInterruptArg(D7, irq_d7, (void*)42, CHANGE);
//attachInterruptArg(D5, irq_d5, (void*)43, CHANGE);
// master / 2.6.2: avg. Latency/us = 12.04
// PR #6047 + Delegate: avg. Latency/us = 11.14
//attachInterrupt(D7, { irq_d7, (void*)42 }, CHANGE);
//attachInterrupt(D5, { irq_d5, (void*)43 }, CHANGE);
// PR #6047 + Delegate: avg. Latency/us = 11.14
//attachInterrupt(D7, std::bind(irq_d7, (void*)42), CHANGE);
//attachInterrupt(D5, std::bind(irq_d5, (void*)43), CHANGE);
// PR #6047 + Delegate: avg. Latency/us = 11.62
start = ESP.getCycleCount();
toggle_d6 ^= HIGH;
digitalWrite(D6, toggle_d6);
}
void loop()
{
if (repeats >= MAXREPEATS)
{
{
esp8266::InterruptLock lock;
Serial.print("avg. Latency/us = ");
Serial.println(1.0 * sum_delay / repeats / ESP.getCpuFreqMHz());
sum_delay = 0;
repeats = 0;
start = ESP.getCycleCount();
toggle_d6 ^= HIGH;
}
digitalWrite(D6, toggle_d6);
}
} |
|
Update: This PR: |
|
Further info, performance of back-to-back IRQs, that is, connecting two pins, and let every change on pin A trigger an interrupt on pin B, where the ISR toggles the state of pin A. Master @ 80MHz: IRQs/s = 136239 Sketch: |
|
For further motivation, here are the results when substituting |
Notable improvement is removal of if(functional), arg casts, which is mighty good thing for the Core's ISR handler speed. With the PR though, doesn't std function's function still need to be explicitly marked as IRAM ? What about lambdas? I've tried some alternative approach at master...mcspr:isr-why |
|
All the compiler limitations with regard to attributes still apply. |
A C++ std library functional/bind/lambdas based quite clean implementation [of the same functionality as in #6055 (closed) ]
Edit: Since May 4 2019, original PR date, a lot has changed, given the time for testing, input from project members, and general refactoring. So here's what this is today:
The interrupt handling code gets updated to use C++ std::function. C-style bindings are preserved as wrappers. This solves issues with leaking of attached arguments on detachInterrupt or re-attaching; is type-safe; makes std::function a first-class citizen for interrupt service routines; has been tested for code paths all being in IRAM, if std::bind is used, but not lambdas, which currently exhibit a compiler weakness in this area.